chore: unify resilience tests and add 401/mid-execution-maintenance scenarios by PeterSchafer · Pull Request #6807 · snyk/cli

PeterSchafer · 2026-05-14T16:39:10Z

Pull Request Submission Checklist

Follows CONTRIBUTING guidelines
Commit messages
are release-note ready, emphasizing
what was changed, not how.
Includes detailed description of changes
Contains risk assessment (Low | Medium | High)
Highlights breaking API changes (if applicable)
Links to automated tests covering new functionality
Includes manual testing instructions (if necessary)
Updates relevant GitBook documentation (PR link: ___)
Includes product update to be announced in the next stable release notes

What does this PR do?

This PR moves existing separate tests into a single test suite for easier overview and maintenance.

Where should the reviewer start?

How should this be manually tested?

What's the product update that needs to be communicated to CLI users?

N/A

Risk assessment (Low | Medium | High)?

Low, changes tests only

snyk-io · 2026-05-14T16:39:37Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

danskmt · 2026-05-15T10:31:55Z

-  it('does not attempt any retries', async () => {
-    await runSnykCLI(`test -d --log-level=trace`, {
-      env: {
-        ...env,
-        // apply a user configured attempts of 10
-        INTERNAL_NETWORK_REQUEST_MAX_ATTEMPTS: '10',
-      },
-    });
-
-    // Count how many times an endpoint was hit
-    const requests = server.getRequests();
-    const actualNetworkAttempts = requests.filter(
-      (r) => r.url.includes('/test-dep-graph') || r.url.includes('/vuln/'),
-    ).length;
-
-    expect(actualNetworkAttempts).toBe(1);
-  });


hmm I think this test is missing in the new file?

I am not fully sure why: the PR description mentions "remove individual test" but the backing ticket does not mention any of that

could you please clarify what you mean? What is the problem you think needs to be addressed?

danskmt · 2026-05-15T10:32:57Z

-      INTERNAL_NETWORK_REQUEST_MAX_ATTEMPTS: '1',
-      INTERNAL_NETWORK_REQUEST_RETRY_AFTER_SECONDS: '1',


Also these two are not present in the new file

Yes, this was intentional!

INTERNAL_NETWORK_REQUEST_RETRY_AFTER_SECONDS has no real meaning in the test
and INTERNAL_NETWORK_REQUEST_MAX_ATTEMPTS is replaced by the official env var SNYK_MAX_ATTEMPTS

danskmt · 2026-05-15T10:36:13Z

+    envOverrides: {
+      SNYK_TIMEOUT_SECS: String(TIMEOUT_SECS),
+    },
+    skip: ['container sbom scratch'],


hmmm do we want to skip this here?

We currently skip these tests for all cases that do not yet behave consistent. This decouples solving of the inconsistency from merging the test suite.

danskmt · 2026-05-15T10:37:38Z

-      // Should send instrumentation data even on timeout
-      const requests = server.getRequests();
-      const instrumentationRequest = requests.find((r) =>
-        r.url?.includes(`/api/hidden/orgs/${orgId}/analytics`),
-      );
-      expect(instrumentationRequest).toBeDefined();


I guess this is also absent in the new file

danskmt · 2026-05-15T10:38:51Z

-  beforeEach(async () => {
-    initialConfig = await getCliConfig();
-    // Set server to delay responses longer than the timeout (10s > 5s timeout)
-    server.setResponseDelay(SERVER_DELAY_MS);
-  });


We don't have beforeEach in the new file, is that intentional?

This is now in the setup function for the scenario.

see here https://github.com/snyk/cli/pull/6807/changes#diff-8216bccbef8ddd0e0637a0383a9428d6a65721fcceebe7e86e4b53943232776cR108

danskmt · 2026-05-15T10:39:53Z

+  'monitor',
+  'whoami',
+  'auth 11111111-2222-3333-4444-555555555555',
+  'sbom --org=11111111-1111-1111-1111-111111111111 --format=cyclonedx1.4+json',


Before it was:

sbom --org=test-org --format=cyclonedx1.4+json

Any reason for changing the org here?

I think this is just about aligning the test with baseEnv defined in lines 195-201

org-id vs org-slugname, internally we use only org-id, if a slugname is specified a lookup needs to happen, which I wanted to avoid in the test.

Do we have any test that tests this lookup happens / should happen?

yes, we have tests in gaf that are specifically focussing in this logic (openbox). The test goal here is error behaviour on a closed box level.

snyk-pr-review-bot · 2026-05-19T08:32:08Z

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Test Logic Error 🟠 [major] The 'mid-execution-maintenance' scenario (Scenario 4) is configured to return success (200) for the first 4 requests and then switch to maintenance mode (503). While the code correctly skips 'whoami' and 'auth' as single-request commands, it fails to skip other short-lived commands like `test`, `monitor`, and `sbom`. In a test environment (scanning the repo root), these commands typically complete in fewer than 5 requests. Consequently, they will receive success for all calls and exit with code 0, which will cause the test assertion to fail as it expects `EXIT_CODES.EX_TEMPFAIL` (11). server.setNextStatusCode(200); server.setNextStatusCode(200); server.setNextStatusCode(200); server.setNextStatusCode(200); server.setGlobalResponse( maintenanceErrorRes, parseInt(maintenanceErrorRes.errors[0].status), ); }, expectedExitCode: EXIT_CODES.EX_TEMPFAIL, expectedErrorCode: 'SNYK-0099', skip: [ 'whoami', // Single-request commands won't hit the failure 'auth', // Single-request commands won't hit the failure 'container monitor scratch', ], Coverage Gap 🟡 [minor] In Scenario 3 ('unauthorized-401'), the `auth` command is explicitly skipped. This is a significant coverage gap because the `auth` command's primary purpose is credential verification. Ensuring that it handles 401 responses gracefully and consistently with other commands is critical for the stated goal of 'Consistent CLI Behavior'. 'auth', // auth doesn't need to
📚 Repository Context Analyzed This review considered 5 relevant code sections from 4 files (average relevance: 1.00) CONTRIBUTING.md (2 segments, lines 1-218) package.json (lines 1-224) dangerfile.js (lines 1-111) src/cli/main.ts (lines 1-236)

PeterSchafer requested review from a team as code owners May 14, 2026 16:39

This comment has been minimized.

Sign in to view

PeterSchafer commented May 14, 2026

View reviewed changes

Comment thread test/acceptance/fake-server.ts Outdated

danskmt reviewed May 15, 2026

View reviewed changes

Comment thread test/jest/acceptance/resilience.spec.ts

danskmt reviewed May 15, 2026

View reviewed changes

PeterSchafer force-pushed the chore/CLI-1494_fault_injection branch from 07a89e6 to a61fdab Compare May 18, 2026 17:01

This comment has been minimized.

Sign in to view

robertolopezlopez approved these changes May 18, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

robertolopezlopez force-pushed the chore/CLI-1494_fault_injection branch from 6e8030d to 084be22 Compare May 19, 2026 07:44

This comment has been minimized.

Sign in to view

robertolopezlopez changed the title ~~chore: add resilience-test suite and remove individual test~~ chore: unify resilience tests and add 401/mid-execution-maintenance scenarios May 19, 2026

chore: add resilience-test suite and remove individual test

8e496d9

PeterSchafer force-pushed the chore/CLI-1494_fault_injection branch from 084be22 to 8e496d9 Compare May 19, 2026 08:29

danskmt approved these changes May 19, 2026

View reviewed changes

PeterSchafer enabled auto-merge May 19, 2026 08:32

PeterSchafer merged commit b5ae18c into main May 19, 2026
9 checks passed

PeterSchafer deleted the chore/CLI-1494_fault_injection branch May 19, 2026 09:33

		INTERNAL_NETWORK_REQUEST_MAX_ATTEMPTS: '1',
		INTERNAL_NETWORK_REQUEST_RETRY_AFTER_SECONDS: '1',

Conversation

PeterSchafer commented May 14, 2026

Pull Request Submission Checklist

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

What's the product update that needs to be communicated to CLI users?

Risk assessment (Low | Medium | High)?

Uh oh!

snyk-io Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danskmt May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

snyk-pr-review-bot Bot commented May 19, 2026

PR Reviewer Guide 🔍

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snyk-io Bot commented May 14, 2026 •

edited

Loading

danskmt May 19, 2026 •

edited

Loading